Conversation
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
feat(pridepy): add pridepy download module
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: ⛔ Files ignored due to path filters (1)
📒 Files selected for processing (4)
🚧 Files skipped from review as they are similar to previous changes (2)
📝 WalkthroughWalkthroughAdds a new Changes
Sequence DiagramsequenceDiagram
participant Workflow
participant PRIDEPY_DOWNLOAD
participant PridepyEnv as "pridepy\n(Conda)"
participant FS as "FileSystem\noutputs"
Workflow->>PRIDEPY_DOWNLOAD: submit meta { id, ... }
alt task.ext.when == null or true
PRIDEPY_DOWNLOAD->>PridepyEnv: create/activate env (pridepy v0.0.14)
PRIDEPY_DOWNLOAD->>PridepyEnv: run pridepy with task.ext.args
PridepyEnv-->>PRIDEPY_DOWNLOAD: downloaded files
PRIDEPY_DOWNLOAD->>FS: write spectra files, optional checksums
else stub mode
PRIDEPY_DOWNLOAD->>FS: create stub .raw and optional checksum
end
PRIDEPY_DOWNLOAD->>PridepyEnv: query installed pridepy version
PridepyEnv-->>PRIDEPY_DOWNLOAD: version string or unknown
PRIDEPY_DOWNLOAD->>FS: write versions.yml
PRIDEPY_DOWNLOAD-->>Workflow: emit (meta, spectra?, checksums?, versions.yml)
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Suggested labels
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Warning Review ran into problems🔥 ProblemsGit: Failed to clone repository. Please run the Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
Up to standards ✅🟢 Issues
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
modules/bigbio/pridepy/tests/nextflow.config (1)
2-2: Avoid potential publishDir collisions if this module grows to multiple processes.Line 2 keeps only the first
_token from the process name, so similarly prefixed process names would publish into the same folder. Using the full normalized process name is safer.Proposed tweak
-process { - publishDir = { "${params.outdir}/${task.process.tokenize(':')[-1].tokenize('_')[0].toLowerCase()}" } -} +process { + publishDir = { "${params.outdir}/${task.process.tokenize(':')[-1].toLowerCase()}" } +}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@modules/bigbio/pridepy/tests/nextflow.config` at line 2, The publishDir currently truncates the process name to only the first '_' token (in the expression on publishDir), which can cause collisions; update the publishDir expression to use the full normalized process name: take the task.process token after the ':' (task.process.tokenize(':')[-1]), convert it to lowercase, and normalize it (replace or collapse non-alphanumeric characters to underscores) instead of tokenizing to the first '_' piece so each unique process gets its own folder under params.outdir.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@modules/bigbio/pridepy/tests/main.nf.test`:
- Around line 23-26: The test currently only asserts process.out.versions; add
an assertion that validates the emitted download_dir output from the workflow
(e.g., assert snapshot(process.out.download_dir).match("download_dir_stub") or
an equivalent existence/structure check) so regressions to the download_dir
output from modules/bigbio/pridepy/main.nf are caught; update the test block
that references process and snapshot to include this download_dir assertion
alongside the existing versions assertion.
---
Nitpick comments:
In `@modules/bigbio/pridepy/tests/nextflow.config`:
- Line 2: The publishDir currently truncates the process name to only the first
'_' token (in the expression on publishDir), which can cause collisions; update
the publishDir expression to use the full normalized process name: take the
task.process token after the ':' (task.process.tokenize(':')[-1]), convert it to
lowercase, and normalize it (replace or collapse non-alphanumeric characters to
underscores) instead of tokenizing to the first '_' piece so each unique process
gets its own folder under params.outdir.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 403d3c1d-5d29-403f-b16f-5b4e0443989b
⛔ Files ignored due to path filters (1)
modules/bigbio/pridepy/tests/main.nf.test.snapis excluded by!**/*.snap
📒 Files selected for processing (5)
modules/bigbio/pridepy/environment.ymlmodules/bigbio/pridepy/main.nfmodules/bigbio/pridepy/meta.ymlmodules/bigbio/pridepy/tests/main.nf.testmodules/bigbio/pridepy/tests/nextflow.config
| then { | ||
| assert process.success | ||
| assert snapshot(process.out.versions).match("versions_stub") | ||
| } |
There was a problem hiding this comment.
Stub test does not validate the download_dir output contract.
Line 25 only checks versions. Since modules/bigbio/pridepy/main.nf (Line 15) also emits download_dir, regressions there can pass unnoticed.
Suggested assertion addition
then {
assert process.success
+ assert snapshot(process.out.download_dir).match("download_dir_stub")
assert snapshot(process.out.versions).match("versions_stub")
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| then { | |
| assert process.success | |
| assert snapshot(process.out.versions).match("versions_stub") | |
| } | |
| then { | |
| assert process.success | |
| assert snapshot(process.out.download_dir).match("download_dir_stub") | |
| assert snapshot(process.out.versions).match("versions_stub") | |
| } |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@modules/bigbio/pridepy/tests/main.nf.test` around lines 23 - 26, The test
currently only asserts process.out.versions; add an assertion that validates the
emitted download_dir output from the workflow (e.g., assert
snapshot(process.out.download_dir).match("download_dir_stub") or an equivalent
existence/structure check) so regressions to the download_dir output from
modules/bigbio/pridepy/main.nf are caught; update the test block that references
process and snapshot to include this download_dir assertion alongside the
existing versions assertion.
fix(pridepy): drop output/ subdir, write to workDir
3 participants
Pull Request
Description
Checklist
main.nfincludes process definitionmeta.ymlincludes complete documentationenvironment.ymlspecifies dependenciesModule Type
Related Issues
Closes #
Summary by CodeRabbit
New Features
Tests